Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make windows fast again (on domain connected machines) #297

Closed
wants to merge 3 commits into from

Conversation

omerbenamram
Copy link

@omerbenamram omerbenamram commented Nov 3, 2019

Hi @Peltoche, thanks for making lsd!

I've looked into #200 and did some profiling, the indeed almost all of the time spent is in GetEffectiveRightsFromAclW when trying to query one of the "Well known" sids.

The problem is that on domain joined machines, windows will try to contact the DC for every file (you can see the calls in the flamegraph to GetDomainInfo), which could take a few hundread miliseconds per file, this PR changes lsd to only query user and group information, which return instantly.

image

There isn't a 1:1 mapping between windows file security conecpts and linux's, and querying this information for an entire folder takes 20-50 seconds, which makes lsd difficult to use.
Personally I find the information for User/Group more than sufficient :)

I could also feature-gate this change if you feel more comfortable with it.

Closes #200.

@lzybkr
Copy link

lzybkr commented Jan 14, 2020

Instead of calling GetEffectiveRightsFromAclW for every file, can we instead call it once per unique sid?

And if that still feels too slow - maybe persist this cache in TEMP and refresh it at most once a day?

@bgianfo
Copy link

bgianfo commented Apr 21, 2020

Any chance of progress being made here?

@AsafMah
Copy link

AsafMah commented Jan 8, 2021

Why isn't this merged already?

@meain
Copy link
Member

meain commented Jan 10, 2021

Hey, sorry that this PR did not get much attention till now. I would like to reopen the PR, but I don't really have any knowledge of how permissions in Windows works.

From what I could understand, "world" is kinda like "everyone else". I would like to know how important of a metric this permission is and how much of an effect this has.

@AsafMah
Copy link

AsafMah commented Jan 10, 2021

I've tried making a version that caches results to "get_acl_access_mask", and while it does help a little, especially with world that only need to be done once,
It is still very very slow compared to a normal dir command.

@meain
Copy link
Member

meain commented Jan 10, 2021

I don't think we should cache it. For a tool like this showing stale/incorrect data might be worse that not showing any.

@sql-sith
Copy link

I hope this PR or something like it can be released as soon as is feasible. I just installed the latest and greatest lsd on a domain-joined Windows laptop. Like many others, I am working from home. If I am not on my company VPN, lsd fails with lsd: .\soapui-settings.xml: The specified domain either does not exist or could not be contacted. (os error 1355). It works fine in WSL but not in pwsh or cmd. See the attached screencap for a couple examples of this.

image

@bgianfo
Copy link

bgianfo commented Jan 11, 2021

I think not querying this data and not displaying the hard coded data on windows is the best option. For every day use it makes more sense to make the tool usable, before supporting features like this on windows.

@sql-sith
Copy link

By the way, I can enumerate the NTFS access permissions of all 866 objects in a directory on my laptop in 10.67 seconds. This is on a work laptop that is a domain member but not attached to the domain, and it is probably not using the fastest available method to do the enumeration.

@meain , you said you don't understand Windows permissions very well, but if you just imagine a Linux system where all the files show a + at the end of their mode/permission block in ls, that's at least a good start. They also have the concept of explicit denial of permissions, which overrides granted permissions, no matter what. And they have the concept of inheritance, which if enabled allows you to change the permissions of an entire directory tree with one modification to the parent.

Significantly, while filesystem objects in NTFS have owners, they do not have any specific group associated with them, and they might or might not have any permissions relating to something similar to "world."

To give some context, this scheme is really flexible, but it is complex enough that Windows doesn't even try to show a summary of permissions. The closest their standard CLI tools get is dir /q from cmd.exe, which displays the owner of each object. Powershell's Get-ChildItem displays a Mode column, but those are object attributes like read-only, archive, hidden, etc., and not Linux-style modes (like 644). Get-ChileItem also has a parameter to list the owner, but there is no way to list permissions with it. What you can do with Get-ChildItem is pipe it to Get-NTFSAccess, which will show you the actual underlying permissions. Here's an example of that for a typical file and a typical directory:

image

So TL;DR - I say if MS doesn't show permissions, why should lsd? Don't include them now if they are causing so much trouble. Later on, maybe somebody (like me?) can circle back and put in a more efficient way to get permissions from Windows. Alternatively, maybe we could consider adding an owner column and an "attributes" block (read-only, etc) like MS does.

@meain
Copy link
Member

meain commented Jan 12, 2021

@sql-sith , thanks for the details and screenshots.

Just hard coding false as the value for "world" permission feels like a hack to me personally. Here is what I think we could do instead.

lsd has a concept of blocks which lets to enable or disable certain blocks of data like date or permission. As of now, we try to load all the metadata for a file when listing even if we don't have to display it. I think we could change that behavior to instead loading only the metadata needed to display the blocks that the user requested for.
Once this is implemented, anyone on a domain connect machine could just disable the permissions block(in the config file). This should help keep the "world" permission for non domain connect machines which does not seem to have this issue. As an added benefit, this might also speed up listings which don't make use of all the blocks.

In future we could probably add an "attributes" column on Windows like @sql-sith suggested.

@sql-sith
Copy link

sql-sith commented Jan 12, 2021

@meain, thanks for the reply! I also thought of the approach of disabling blocks but when I commented out the permission block, it still was obviously computing it. So if the behavior was changed to not look up this information if the block will not be displayed, that would be better.

However, this will not be an obvious fix to most users, so I would suggest turning it off by default (at least on Windows PCs in a domain), or showing an extremely obvious message during installation allowing the user to shut off permissions.

Regarding your comment about hard-coding false for world permissions, I think my previous post may have been inaccurate or misleading. What I meant to say is that of the three owner/group/world permission modes in Linux, the only one you have with certainty in Windows is owner. There may be 0, 1, or many groups, and some of those groups have a "world" vibe to them (public, guests, authenticated users, etc.).

So yes, I was suggesting not showing permissions for any group in Windows, not just world. In fact, we could show owner and world permissions if we pick one group to substitute for world, such as Users. Then we could get rwx for both the owner and for that group (ignoring other permissions Windows ACEs can contain). However, I can't think of any reasonable substitute for group permissions that isn't ... idunno, fraud? :-) Maybe someone else can.

@meain
Copy link
Member

meain commented Jan 13, 2021

From what I could understand, 2 blocks don't translate over (permissions, group) as windows might have multiple groups and there is no other.

Regarding disabling permission block(which if we were to do, we should disable both group and permission blocks) on Windows, even thought it does not exactly translate over, I think we will keep the current default blocks for now so as to not change anything for existing users as this specific issue only seems to affect domain connected machines. That said, we could add info about this in the README, and in any error messages when trying to retrieve permissions on Windows machines.

@sql-sith
Copy link

From what I could understand, 2 blocks don't translate over (permissions, group) as windows might have multiple groups and there is no other.

@meain, that is exactly right. Also, I think that what you are proposing is perfect for now. Keeping the output looking similar is a good thing. I say go for it. :-) I can't help right now because work is too busy but might be able to later ... although maybe I should check what language this is before volunteering.

@AsafMah
Copy link

AsafMah commented Jan 14, 2021

Maybe add a flag to not retrieve this data?
Then people with domain machines can just put it in their config files

@meain
Copy link
Member

meain commented Jan 14, 2021

@AsafMah Yes, that is the plan. The idea is that we will only retrieve only the info needed to display the output. So if the user specifies blocks in the config to not have permissions the code will skip trying to fetch that information.

@vvuk
Copy link

vvuk commented Feb 9, 2021

This affects domain-connected machines the most, but it's still slow even on machines not part of a domain. In a directory with ~100 files (mostly .dll/.pdb files), lsd master (today's) takes 0.65s to produce a listing. With this patch, it takes 0.44s, which is a significant improvement.

Note that 0.44s is still extremely slow; I haven't done any profiling yet to figure out why. Running bash -c ls (running ls through WSL) takes 0.07s. Running powershell Show-ChildItemColumns takes 0.01s. lsd should be able to hit at least WSL speeds; half a second is painful enough that it's not really usable as a replacement on windows.

@vvuk
Copy link

vvuk commented Feb 9, 2021

It's GetEffectiveRightsFromAclW causing the slowdown. From a trace:
image

There was a comment earlier about not querying for data that isn't going to be displayed; that sounds like the right approach to solving this, though in the interim it might be helpful to just have a way to not query any of this on windows.

@vvuk
Copy link

vvuk commented Feb 9, 2021

I just added a quick and dirty way to disable the ownership/permissions checks on windows in #484. With that feature, lsd becomes as fast as any other tool (down to 0.04s for the dir above -- so twice as fast as WSL, and within range of the speed-of-light no-process-spawn powershell cmdlet)

@sql-sith
Copy link

sql-sith commented Feb 9, 2021

I just added a quick and dirty way to disable the ownership/permissions checks on windows in #484. With that feature, lsd becomes as fast as any other tool (down to 0.04s for the dir above -- so twice as fast as WSL, and within range of the speed-of-light no-process-spawn powershell cmdlet)

This is great news! I came out to see if anyone had worked on this yet. I'll give it a spin. Feel free to hit me up directly if/when you want to find fast ways to enumerate ACLs in Windows (not that you'll need my help, but just in case).

@sql-sith
Copy link

@vvuk , I am not a Rustacean, so I have a really basic question. After building the executable from your PR, when I use it on a domain-connected computer it is still very slow, unfortunately. Is there anything I need to do to enable this feature? Thanks a lot for working on this.

@vvuk
Copy link

vvuk commented Feb 10, 2021

Yep, you need to build with cargo build --release --features no-windows-permissions. The feature is not enabled by default.

@sql-sith
Copy link

Yep, you need to build with cargo build --release --features no-windows-permissions. The feature is not enabled by default.

See? I know nothing about rust. Or rather, now I know one thing. :) Thanks!

@AsafMah
Copy link

AsafMah commented Feb 10, 2021

Also see this PR: #475
Where I added not getting permissions if the block isn't requested.

@meain
Copy link
Member

meain commented Feb 13, 2021

This is what I have in mind so far. For now, as a stopgap solution we can have permission fetching disabled in windows under a feature flag (#484). The issue with #475 is that we need the permissions to color file names even if they are not explicitly requested.
In the long term, the idea is to either speed up the actual fetch (ref : #475 (comment)) or show permission on windows in a more windowsy way (ref: #297 (comment)) for just the one user (which I guess should be relatively faster to fetch).

@meain
Copy link
Member

meain commented Mar 24, 2021

Closing this. #484 seems to be better for now.

@meain meain closed this Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lsd is slow Windows
7 participants